Adding fvdb-core#31550
Conversation
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/fvdb-core/meta.yaml:
For recipes/fvdb-core/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19655854129. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/fvdb-core/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19656331531. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19657321507. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/fvdb-core/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/21971138885. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the staged-recipes linter and I found some lint. It looks like some changes were made outside the If these changes are intentional (and you aren't submitting a recipe), please add a File-specific lints and/or hints:
|
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
This reverts commit a01c27e. Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
|
@conda-forge/help-python, ready for review! This pytorch plugin requires CUDA 12.9. In the current 12.6 ci_support configuration, the conda environments for the build will fail to resolve and so I had to modify the ci_support config to be able to run properly which you can see running here: https://github.com/conda-forge/staged-recipes/runs/56342507840 And I reverted that change for the current state of the PR. Unfortunately the machine it's running on seems to run out of memory partway through the build, though I can confirm the recipe runs successfully on my local machine with Thank you |
|
@conda-forge-admin, please ping team |
|
Hi! This is the friendly automated conda-forge-webservice. I was asked to ping @conda-forge/staged-recipes and so here I am doing that. |
|
To help direct your pull request to the best reviewers, please mention a topic-specifc team if your recipe matches any of the following: conda-forge/help-c-cpp, conda-forge/help-cdts, conda-forge/help-go, conda-forge/help-java, conda-forge/help-julia, conda-forge/help-nodejs, conda-forge/help-perl, conda-forge/help-python, conda-forge/help-python-c, conda-forge/help-r, conda-forge/help-ruby,or conda-forge/help-rust. Thanks! |
|
@conda-forge/help-python-c, ready for review! This pytorch plugin requires CUDA 12.9. In the current 12.6 ci_support configuration, the conda environments for the build will fail to resolve and so I had to modify the ci_support config to be able to run properly which you can see running here: https://github.com/conda-forge/staged-recipes/runs/56342507840 And I reverted that change for the current state of the PR. Unfortunately the machine it's running on seems to run out of memory partway through the build, though I can confirm the recipe runs successfully on my local machine with build-locally.py. Thank you |
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/fvdb-core/meta.yaml:
For recipes/fvdb-core/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/21971188651. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/fvdb-core/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/22127823460. Examine the logs at this URL for more detail. |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
|
||
|
|
||
| setup_parallel_build_jobs | ||
| $PYTHON -m pip install . --no-deps --no-build-isolation -vv |
There was a problem hiding this comment.
| $PYTHON -m pip install . --no-deps --no-build-isolation -vv | |
| export CMAKE_GENERATOR=Ninja | |
| $PYTHON -m pip install \ | |
| --no-deps \ | |
| --no-build-isolation \ | |
| -vv \ | |
| -C 'skbuild.ninja.make-fallback=false' \ | |
| . |
The latest build is failing like this:
-- Configuring done (68.4s)
CMake Error in .cpm_cache/vulkanheaders/6fc921fffac75636a2e70c97404ed7ccbe1cfabf/CMakeLists.txt:
The target named "Vulkan-Module" has C++ sources that may use modules, but
modules are not supported by this generator:
Unix Makefiles
Modules are supported only by Ninja, Ninja Multi-Config, and Visual Studio
generators for VS 17.4 and newer. See the cmake-cxxmodules(7) manual for
Let's try forcing the use of ninja.
There was a problem hiding this comment.
Ha looks like you beat me to it: 5bb4a7e
I think you still might want to also set skbuild.ninja.make-fallback=false to be sure we get a loud error if ninja can't be found (instead of silently falling back to make). But not too important either way.
…en ninja isn't available Added -Wno-error=stringop-overflow to fix false positive in GCC 14 Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
… present Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Jonathan! Also thanks James for reviewing 🙏
Had a couple questions on the recipe. Mostly minor. And some simplifications
Think this is coming along nicely 🙂
| - cuda-version {{ cuda_compiler_version }} | ||
| - cuda-nvcc-impl >=12.8 | ||
| - cuda-nvrtc-dev | ||
| - cuda-nvtx-dev |
There was a problem hiding this comment.
It looks like this isn't getting detected by CMake
-- CPM: Adding package nvtx3@3.1.0 (v3.1.0-c-cpp)
-- get_nvtx.cmake: Forced nvtx3_dir to CACHE: /home/conda/staged-recipes/build_artifacts/fvdb-core_1771020205707/work/build/cp312-cp312-linux_x86_64-Release/_deps/nvtx3-src/include (from CPM source)
So we may need to adjust the CMake options so it is found
Alternatively we can use the nvtx-c package where we have confirmed CMake detection works
If we need to push more changes here, we can include one of these fixes. Otherwise it is ok to handle in the feedstock
There was a problem hiding this comment.
I think I'd prefer to handle it in the feedstock just because we're gearing up for a new release in early March where I can make these build changes (as well as some of the other changes to support the conda-forge build better) and not have to patch the old 0.3.0 release.
There was a problem hiding this comment.
Have filed this as issue: conda-forge/fvdb-core-feedstock#3
| - {{ compiler('cxx') }} | ||
| - {{ compiler('cuda') }} | ||
| - cuda-version {{ cuda_compiler_version }} | ||
| - cuda-command-line-tools |
There was a problem hiding this comment.
I believe just nvcc, I thought this was the appropriate way to pull in the CUDA compiler. I had found at some point that {{ compiler('cuda') }} wasn't enough
There was a problem hiding this comment.
Yeah {{ compiler('cuda') }} should be enough for nvcc
There was a problem hiding this comment.
Have filed this as issue: conda-forge/fvdb-core-feedstock#2
If we wanted to do this, we could adjust the The reason being we treat the CUDA version as a matrix variable (like Python). So we either build or don't build for a specific CUDA version. So within any specific build it is pinned to a version. It isn't being solved for like any other dependency Hope that makes more sense. Happy to discuss more |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
|
It looks like we got through the tests! 🥳 The tests that fail reference the absence of a GPU. Is there an easy way to skip those tests? |
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Yes this is great, thanks @jakirkham! I pushed up a change to the test script to check for the presence of GPU However, with some of the other recipe changes, I'm now getting these overlinking errors on packaging. I believe some of the lines I removed/changed in my last commit were changes I discovered earlier I needed to resolve these overlinking errors. |
|
Thanks Jonathan! Credit goes to you and James for getting this into shape 🙏 Based on the CI overlinking error, it looks like we are linking to Does that sound correct? Should we be depending on |
Well @jakirkham we do use |
| - gitpython | ||
| - git | ||
| - parameterized | ||
|
|
There was a problem hiding this comment.
Anticipating more comments for the cudnn question, could we use a thread?
I see the following in the CMake configure logs:
-- USE_CUDNN is set to 0. Compiling without cuDNN support
But then later see cudnn_frontend pulled in via CPM:
-- CPM: Adding package cudnn_frontend@1.3.0 (v1.3.0)
-- Populating cudnn_frontend
-- Configuring done (0.0s)
-- Generating done (0.0s)
-- Build files have been written to: /home/conda/staged-recipes/build_artifacts/fvdb-core_1771202868458/work/build/cp312-cp312-linux_x86_64-Release/_deps/cudnn_frontend-subbuild
[1/9] Creating directories for 'cudnn_frontend-populate'
[1/9] Performing download step (git clone) for 'cudnn_frontend-populate'
Cloning into 'cudnn_frontend-src'...
HEAD is now at 1b0b5ea cudnn frontend v1.3 release notes. (#72)
[2/9] Performing update step for 'cudnn_frontend-populate'
-- Already at requested tag: v1.3.0
[3/9] No patch step for 'cudnn_frontend-populate'
[5/9] No configure step for 'cudnn_frontend-populate'
[6/9] No build step for 'cudnn_frontend-populate'
[7/9] No install step for 'cudnn_frontend-populate'
[8/9] No test step for 'cudnn_frontend-populate'
[9/9] Completed 'cudnn_frontend-populate'
Either way, could the libcudnn.so dependency be coming through transitively from libtorch.so?
conda create \
--name torch-test \
--yes \
'libtorch 2.9.1 cuda129_mkl_hf3b6726_305'
source activate torch-test
find ${CONDA_PREFIX} -name 'libtorch.so*' -exec ldd {} \+...
libcudnn.so.9 => /home/jlamb/miniforge3/envs/torch-test/lib/././libcudnn.so.9 (0x000078d7fe200000)
...
And if it is, shouldn't the pytorch =*=*{{ torch_proc_type }}* dependency in run: be enough to pull in libtorch and therefore libcudnn? 🤔
I will try to look into this soon.
There was a problem hiding this comment.
I would have thought the same @jameslamb
cudnn_frontend is header-only afaik. But I do see that we have dynamically linked libcudnn directly:
❯ readelf -d libfvdb.so | grep NEEDED
0x0000000000000001 (NEEDED) Shared library: [libtorch.so]
0x0000000000000001 (NEEDED) Shared library: [libc10.so]
0x0000000000000001 (NEEDED) Shared library: [libc10_cuda.so]
0x0000000000000001 (NEEDED) Shared library: [libcudnn.so.9]
0x0000000000000001 (NEEDED) Shared library: [libtorch_cpu.so]
0x0000000000000001 (NEEDED) Shared library: [libtorch_cuda.so]
0x0000000000000001 (NEEDED) Shared library: [libcudart.so.12]
0x0000000000000001 (NEEDED) Shared library: [libstdc++.so.6]
0x0000000000000001 (NEEDED) Shared library: [libm.so.6]
0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1]
0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
0x0000000000000001 (NEEDED) Shared library: [ld-linux-x86-64.so.2]
Should I add libcudnn to reqs/run as the error message implies or should I add libcudnn-dev to reqs/host like the other CUDA libraries?
There was a problem hiding this comment.
I added the requirement to the run requirements and now we're back to passing checks!
There was a problem hiding this comment.
Ok great! I think it's fine for now to just take on the dependency and to investigate removing it later in the feedstock.
By the way, if you want (no pressure!) I'd be happy to join as a maintainer for this recipe if you'd like another set of hands to keep things moving.
There was a problem hiding this comment.
That would be amazing, thank you! Someone who can lend a hand with knowledge in this domain would be fantastic
There was a problem hiding this comment.
Ok sure, thanks! You could add me in the recipe-maintainers: section in the recipe here, or we can do it later in the feedstock, following https://conda-forge.org/docs/maintainer/updating_pkgs/#updating-the-maintainer-list
Either is fine.
There was a problem hiding this comment.
Filed an issue to follow up on cuDNN: conda-forge/fvdb-core-feedstock#4
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
jameslamb
left a comment
There was a problem hiding this comment.
We identified a couple of other things to be done in the feedstock (tuning dependencies, improving build times, covering more Python versions) but I'm happy with the current state of this. Thanks for working through all the feedback!
|
Thanks Jonathan and James! 🙏 Sounds like a plan |
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).